-
Notifications
You must be signed in to change notification settings - Fork 340
change: Drop Python 3.5 support #542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
3d45c1d
to
dc8e2cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I only had one suggestion.
Have you thought about how to release/version this? In the past we have bumped major version numbers when we discontinued support for a platform version. But GCP is following a more relaxed policy with respect to that (albeit unofficially).
tests/testutils.py
Outdated
# Fallback for Python 3.5 | ||
from _pytest.monkeypatch import MonkeyPatch | ||
return MonkeyPatch() | ||
return MonkeyPatch() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just return pytest.MonkeyPatch()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like pytest.MonkeyPatch()
works for pytest
6.2 and up. Our CIs use the latest version so I think it would be safe to update this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just set pytest version to 6.2 and up in requirements.txt file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! Updated.
Thanks! I would prefer we do a major version bump to be consistent with our previous releases. Having said that, if we would rather align our releases with GCP policies a minor version bump would not be a big issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
My personal preference is to bump the major version as well. GCP policy on this issue is still a work in progress, and might take a while to materialize. In the meantime, I'd be ok with bumping the major version for this change.
Sounds good! Let's do a major version bump for this then. Thanks! |
RELEASE NOTE: Dropped support for Python 3.5. Developers should to use Python 3.6 or higher when deploying the Admin SDK.